lzham: Add cci.20220103 #13646
lzham: Add cci.20220103 #13646
Conversation
|
|
This comment has been minimized.
This comment has been minimized.
partiallyderived
force-pushed
the
master
branch
from
c642d34 to
f4e3f4b
Compare
This comment has been minimized.
This comment has been minimized.
partiallyderived
force-pushed
the
master
branch
from
f4e3f4b to
fd71d8a
Compare
This comment has been minimized.
This comment has been minimized.
partiallyderived
force-pushed
the
master
branch
from
fd71d8a to
987c996
Compare
This comment has been minimized.
This comment has been minimized.
partiallyderived
force-pushed
the
master
branch
from
987c996 to
b774bc7
Compare
This comment has been minimized.
This comment has been minimized.
partiallyderived
force-pushed
the
master
branch
from
b774bc7 to
9677726
Compare
This comment has been minimized.
This comment has been minimized.
partiallyderived
force-pushed
the
master
branch
from
9677726 to
5d142e8
Compare
This comment has been minimized.
This comment has been minimized.
partiallyderived
force-pushed
the
master
branch
from
5d142e8 to
b0ec2f0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
partiallyderived
force-pushed
the
master
branch
from
ee9b3ae to
752acd2
Compare
partiallyderived
force-pushed
the
master
branch
from
752acd2 to
13050e3
Compare
This comment has been minimized.
This comment has been minimized.
partiallyderived
force-pushed
the
master
branch
from
13050e3 to
1177828
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
partiallyderived
force-pushed
the
master
branch
from
2833581 to
92fe3a0
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
The big "update" patch is a blocker https://github.com/conan-io/conan-center-index/blob/41adf57553fa3ab9088462d390a2c444fdc7f052/docs/adding_packages/sources_and_patches.md#policy-about-patching
I made a few small suggestions as well
This is a really good start!
recipes/lzham/all/conandata.yml
Outdated
| - patch_file: "patches/commits-1.0.0.patch" | ||
| patch_description: 'Updates code to latest commit for the repo |
There was a problem hiding this comment.
Oof, this is going to be a very tough sell 🙊
Thats a massive patch....
Couple suggestions for you to get this to move forward
- Ask upstream to make a new release
- Contribute your patches to the project 🙏 this would be amazing for everyone
The other options is we do have "unofficial releases" https://github.com/conan-io/conan-center-index/blob/master/docs/faqs.md#what-version-should-packages-use-for-libraries-without-official-releases and that would be something you could add in addition to the official one
There was a problem hiding this comment.
The first option is ideal and I've made an issue for it. Unfortunately the maintainers are pretty unresponsive so I doubt it will come to pass haha. I think the second option doesn't apply as this code is already part of the upstream, though maybe I misunderstood. I made pull requests for my other patches where they did not already exist.
I think it is likely that I will go for the "unofficial releases" option
There was a problem hiding this comment.
Also, is it acceptable if I supported only the unofficial release? I ask because it contains many portability fixes, and the official would basically have to be patched to include the fixes anyway if I include it
There was a problem hiding this comment.
I think it makes sense in this case given how much differences there are
| def generate(self): | ||
| if is_msvc(self): | ||
| tc = MSBuildToolchain(self) |
There was a problem hiding this comment.
Why not build all platform with CMake? https://github.com/richgel999/lzham_codec/blob/master/CMakeLists.txt
There was a problem hiding this comment.
Thanks for the review! Part of the reason is that the CMakeLists configure compiler arguments that MSVC does not understand, leading to errors like
cl : command line error D8021: invalid numeric argument '/Wextra' [C:\Users\Babbo\.conan\data\lzham\1.0.0\_\_\build\5a
61a86bb3e07ce4262c80e1510f9c05e9b6d48b\build\lzhamdecomp\lzhamdecomp.vcxproj]
Another reason is that I suspect that the MSVC project is "fine-tuned" for Windows and it is the recommended way to compile for Windows, so I believe it would be less optimal to make a patch adapt the CMakeLists so that MSVC is handled, not to mention a bit of a pain!
This comment has been minimized.
This comment has been minimized.
partiallyderived
requested review from
prince-chrismc
and removed request for
franramirez688 and
uilianries
Conan v1 pipelineAll green in build 22 (
|
* lzham: Add 1.0.0 * Respond to comments * Fix lint error
Specify library name and version: lzham/cci.20220103
Reason for creating PR: I'm working on a project in C++ which will benefit from having great compression and faster decompression, for which I've decided LZHAM is a good library. Resolves #13500
Several patches are applied to make the code more portable. An unofficial release was selected as the only official release is quite old and missing many portability fixes, which would need to be patched in anyway.